Documentation fixes with some linting #219

darrenmeehan · 2020-04-28T12:32:32Z

Description of changes:

Fixed documentation in README.md to work with existing code.
Update awscli commands to work with version 2.
Ensure files built when following README.md are ignored from git
Remove Travis button as it's no longer used.
Resolves AWS CLI Instructions README Typo #84

Some code changes. I can revert this to make this PR cleaner if deemed more appropriate.

Fixed minor typos in docstring.
Minor linting.

By submitting this pull request

I confirm that my contribution is made under the terms of the Apache 2.0 license.
I confirm that I've made a best effort attempt to update all relevant documentation.

Remove Travis button as it's no longer used.

softprops · 2020-04-28T20:43:48Z

README.md


 ```bash
 $ aws lambda invoke --function-name rustTest \
+  --cli-binary-format raw-in-base64-out \


It might be worth calling out you need the aws v2 cli for this to work. Some are using that but as it's relatively new, many are not.

For clearer context https://docs.aws.amazon.com/cli/latest/userguide/cliv2-migration.html#cliv2-migration-binaryparam

It might be worth calling out you need the aws v2 cli for this to work. Some are using that but as it's relatively new, many are not.

Great call. Should I add the invokcation command for both versions? Or highlight the need for --cli-binary-format with v2? I'm learning towards the latter as it's less verbose.

I've went with the following but happy to change! 56a745f

Yeah, let’s prefer version two of the CLI. I’d be surprised if the audience for this crate aren’t also early adopters of the newer CLI.

My apologies it was late when I made the above commit 56a745f. It should say most users are using v1.

I think as it stands the README.md does a good job of documenting the command for both v1 & v2. Is there any change you would like here @davidbarsky ?

Oh, yeah. No change then. Thanks for doing this!

softprops · 2020-04-28T20:50:40Z

lambda/Cargo.toml

 [package]
 name = "lambda"
 version = "0.1.0"
 authors = ["David Barsky <[email protected]>"]


I might drop the "client" part of this. That's an implementation detail most users won't likely care or care to know about. Just calling this "AWS Lambda Rust runtime" might get the point across and be a little bit more inline with https://docs.aws.amazon.com/lambda/latest/dg/lambda-runtimes.html

Thanks for the feedback. I've addressed this in f3337db

This is a closer fit to the convention for runtimes across different languages.

Most users are still using v2 of the cli. Let's keep things simple for them.

lambda/src/lib.rs

Also expanded on comment Co-Authored-By: David Barsky <[email protected]>

davidbarsky · 2020-04-29T15:28:25Z

lambda/src/lib.rs

+//!    be placed on an asynchronous main function. However, asynchronous main
+//!    functions are not legal valid Rust, which means that a crate like
 //!    [Runtime](https://github.com/rustasync/runtime) must be used. A main function


(Sorry for the back and forth on this; I reviewed this before my coffee hit.)

Can we reword this to say something along the lines of "Tokio is required; we suggest using #[tokio::main]"?

No worries at all. I'll get back to this in about an hour once I've food.

Fire away with any requests :)

Thanks! I only noticed this docs issue now since I wrote that; the two big issues are:

runtime is deprecated.

This crate has a hard Tokio dependency (for now). Cross-runtime support is too hard and has too many open design questions, and until Rusoto supports runtimes other than Tokio, I don't think its worth attempting to support other runtimes in this crate.

I'm not totally familiar with tokio but I've attempted to clear this section up. Anything you would like changed? Sorry if I'm misunderstanding something!

davidbarsky

One last comment; then I'll merge this in. Thanks!

darrenmeehan · 2020-04-29T16:04:40Z

@davidbarsky Could this PR be linked with the following issue please? #84

davidbarsky · 2020-04-29T16:11:39Z

Yup! If you’re to add a resolves [issue number] to the description of this PR, it’ll link and close that issue.

darrenmeehan · 2020-05-02T15:05:39Z

Hi @davidbarsky is there anything you would like changed?

davidbarsky · 2020-05-06T14:06:15Z

@darrenmeehan No, this looks great! Sorry for the delay and thanks for the fix!

darrenmeehan · 2020-05-06T18:49:14Z

Awesome thank you for the quick feedback. Looking forward to contributing some more.

darrenmeehan and others added 8 commits April 28, 2020 13:26

Fix minor typos

c1e71e2

Avoid long literal lacking separators

1180ed1

Add description

6f80780

Update README to use hello world example

1e250dd

Update cli changes to work with aws cli v2

f2dd59c

Ignore outputs from follow instructions

0d2872e

Remove Travis button as it's no longer used.

8a76469

Merge pull request #1 from Darrenmeehan/patch-1

acce9a9

Remove Travis button as it's no longer used.

softprops reviewed Apr 28, 2020

View reviewed changes

darrenmeehan added 3 commits April 28, 2020 22:33

Remove client from description

f3337db

This is a closer fit to the convention for runtimes across different languages.

Capitalise runtime in description

6bb60b9

Document --cli-binary-format seperately

56a745f

Most users are still using v2 of the cli. Let's keep things simple for them.

softprops approved these changes Apr 28, 2020

View reviewed changes

darrenmeehan mentioned this pull request Apr 29, 2020

AWS CLI Instructions README Typo #84

Closed

davidbarsky reviewed Apr 29, 2020

View reviewed changes

lambda/src/lib.rs Outdated Show resolved Hide resolved

Fixed name for macro

1ef54a8

Also expanded on comment Co-Authored-By: David Barsky <[email protected]>

darrenmeehan changed the title ~~Documentation Fixes with some linting~~ Documentation fixes with some linting Apr 29, 2020

davidbarsky reviewed Apr 29, 2020

View reviewed changes

davidbarsky approved these changes Apr 29, 2020

View reviewed changes

darrenmeehan added 3 commits April 29, 2020 17:27

Fix typo

00f59db

Highlight tokia requirement and improve clarity

d508b92

Address whitespace issue

7dea464

davidbarsky merged commit 8f026a0 into awslabs:master May 6, 2020

Documentation fixes with some linting #219

Documentation fixes with some linting #219

Uh oh!

Conversation

darrenmeehan commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darrenmeehan Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidbarsky Apr 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidbarsky left a comment

Choose a reason for hiding this comment

Uh oh!

darrenmeehan commented Apr 29, 2020

Uh oh!

davidbarsky commented Apr 29, 2020

Uh oh!

darrenmeehan commented May 2, 2020

Uh oh!

davidbarsky commented May 6, 2020

Uh oh!

darrenmeehan commented May 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

darrenmeehan commented Apr 28, 2020 •

edited

Loading

darrenmeehan Apr 28, 2020 •

edited

Loading

davidbarsky Apr 29, 2020 •

edited

Loading